Push down COALESCE in PostgreSQL connector#11535
Conversation
|
@ebyhr can you please rebase? |
4dff92a to
5bb372b
Compare
|
@findepi Rebased on upstream. |
findepi
left a comment
There was a problem hiding this comment.
"Translate COALESCE to connector expressions"
There was a problem hiding this comment.
Yes,I will note it down to unify that after those PRs are merged.
5bb372b to
22099ed
Compare
22099ed to
c77604c
Compare
There was a problem hiding this comment.
TryExpression is desugared to try function and it's marked as non-deterministic.
We don't push non-deterministic predicates down into the connector, however, they are handled separately from non-translatable ones.
Thus, it's a wrong change, as it changes the semantics of the test case.
There was a problem hiding this comment.
Changed to SearchedCaseExpression though I have a local patch to support it as connector expressions. Do you have other suggested expressions?
There was a problem hiding this comment.
See #11511 (comment) and #11544 (comment)
I don't have a plan for testing things. @martint do you have one?
cc @wendigo @hashhar
c77604c to
6810da6
Compare
6810da6 to
1cd9346
Compare
1cd9346 to
d4722a4
Compare
Yes, coalesce is described by the SQL spec as syntactic sugar for a CASE expression. |
|
If i remember correctly, the E.g. this should not fail similar to #11699 (comment), this means this should not be modeled as a function call. OTOH, modeling @martint what about we model |
d4722a4 to
0d80cff
Compare
|
(Rebased on upstream to resolve conflicts) |
@martint and I talked about this and because However, lambdas support is not a trivial thing, and we don't think we're currently ready to work on this yet. |
|
Thanks for your explanation. Let me close this PR. |
0d80cff to
72651ec
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughA new Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteCoalesce.java (2)
38-46: Simplify pattern: the capture is unnecessary.The
CALLcapture is not needed since thecallparameter passed torewrite()is the same matchedCallobject. You can remove the capture and usecalldirectly in the loop.♻️ Suggested simplification
public class RewriteCoalesce implements ConnectorExpressionRule<Call, ParameterizedExpression> { - private static final Capture<Call> CALL = newCapture(); private final Pattern<Call> pattern; public RewriteCoalesce() { this.pattern = call() - .with(functionName().matching(name -> name.equals(COALESCE_FUNCTION_NAME))) - .capturedAs(CALL); + .with(functionName().equalTo(COALESCE_FUNCTION_NAME)); }Also update the imports to remove
CaptureandnewCaptureif no longer used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteCoalesce.java` around lines 38 - 46, Remove the unnecessary Capture and simplify the pattern in RewriteCoalesce: delete the static CALL and its newCapture() usage, change the pattern to just call().with(functionName().matching(name -> name.equals(COALESCE_FUNCTION_NAME))), and in the rewrite(Call call, ...) implementation use the provided call parameter instead of using CALL.get(). Also remove unused imports Capture and newCapture from the file.
57-72: Remove redundant verify and usecallparameter directly.
- Line 61 uses
captures.get(CALL).getArguments()butcall.getArguments()is equivalent and simpler.- The verify at line 71 is redundant—if line 57 passes with N arguments, the loop will produce exactly N rewritten arguments (or return early on failure).
♻️ Suggested simplification
`@Override` public Optional<ParameterizedExpression> rewrite(Call call, Captures captures, RewriteContext<ParameterizedExpression> context) { verify(call.getArguments().size() >= 2, "Function 'coalesce' expects more than or equals to two arguments"); ImmutableList.Builder<String> rewrittenArguments = ImmutableList.builderWithExpectedSize(call.getArguments().size()); ImmutableList.Builder<QueryParameter> parameters = ImmutableList.builder(); - for (ConnectorExpression expression : captures.get(CALL).getArguments()) { + for (ConnectorExpression expression : call.getArguments()) { Optional<ParameterizedExpression> rewritten = context.defaultRewrite(expression); if (rewritten.isEmpty()) { return Optional.empty(); } rewrittenArguments.add(rewritten.get().expression()); parameters.addAll(rewritten.get().parameters()); } List<String> arguments = rewrittenArguments.build(); - verify(arguments.size() >= 2, "Function 'coalesce' expects more than or equals to two arguments"); return Optional.of(new ParameterizedExpression("COALESCE(%s)".formatted(join(",", arguments)), parameters.build())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteCoalesce.java` around lines 57 - 72, In RewriteCoalesce, simplify the argument rewriting loop by iterating over call.getArguments() instead of captures.get(CALL).getArguments(), set ImmutableList.builderWithExpectedSize(call.getArguments().size()) for rewrittenArguments, and remove the redundant verify(arguments.size() >= 2) check at the end (the initial verify on call.getArguments().size() already guarantees the count or early-return on rewrite failure); keep the existing behavior of returning Optional.empty() on rewrite failure and constructing the final ParameterizedExpression("COALESCE(%s)".formatted(join(",", arguments)), parameters.build()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteCoalesce.java`:
- Around line 38-46: Remove the unnecessary Capture and simplify the pattern in
RewriteCoalesce: delete the static CALL and its newCapture() usage, change the
pattern to just call().with(functionName().matching(name ->
name.equals(COALESCE_FUNCTION_NAME))), and in the rewrite(Call call, ...)
implementation use the provided call parameter instead of using CALL.get(). Also
remove unused imports Capture and newCapture from the file.
- Around line 57-72: In RewriteCoalesce, simplify the argument rewriting loop by
iterating over call.getArguments() instead of captures.get(CALL).getArguments(),
set ImmutableList.builderWithExpectedSize(call.getArguments().size()) for
rewrittenArguments, and remove the redundant verify(arguments.size() >= 2) check
at the end (the initial verify on call.getArguments().size() already guarantees
the count or early-return on rewrite failure); keep the existing behavior of
returning Optional.empty() on rewrite failure and constructing the final
ParameterizedExpression("COALESCE(%s)".formatted(join(",", arguments)),
parameters.build()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8b37cf2-38be-4aea-bdc7-a893f1096874
📒 Files selected for processing (4)
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteCoalesce.javaplugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.javaplugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.javaplugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java
72651ec to
0142908
Compare
|
|
||
| List<String> arguments = rewrittenArguments.build(); | ||
| verify(arguments.size() >= 2, "Function 'coalesce' expects more than or equals to two arguments"); | ||
| return Optional.of(new ParameterizedExpression("COALESCE(%s)".formatted(join(",", arguments)), parameters.build())); |
There was a problem hiding this comment.
is this lazy eval for PostgreSQL too?
We don't seem to have any test coverage for this.
Description
Push down
COALESCEin PostgreSQL connectorRelease notes